Enhance conditions by peter279k · Pull Request #3 · brick/std

peter279k · 2018-12-01T07:38:19Z

Changed log

To reduce nested condition, using the ?: operator to do that.
Using the ?? syntax to reduce conditions.
Using the error Exception first to enhance condition logic.

coveralls · 2018-12-01T07:40:02Z

Coverage decreased (-0.1%) to 94.427% when pulling 7fee5ac on peter279k:enhance_condition into b8acd4b on brick:master.

BenMorel · 2018-12-01T10:04:11Z

src/Iterator/CsvJsonFileIterator.php

+        $this->handle = (is_resource($file) ? $file : @ fopen($file, 'rb'));
+
+        if ($this->handle === false) {
+            throw new \InvalidArgumentException('Cannot open file for reading: ' . $file);


What do you mean about this comment?

I don't like the null coalesce operator here, as it also allows for undefined variable without notice. This operator is equivalent to isset($values) ? $values : [], not strictly equivalent to the original code.

This condition is not related to the null coalesce operator.

I think this change is proper approach.

BenMorel · 2018-12-01T10:06:20Z

src/ObjectArrayStorage.php

        $values = $this->storage->get($object);

-        return ($values === null) ? [] : $values;
+        return $values ?? [];


I don't like the null coalesce operator here, as it also allows for undefined variable without notice. This operator is equivalent to isset($values) ? $values : [], not strictly equivalent to the original code.

BenMorel · 2018-12-01T10:07:16Z

src/ObjectStorage.php

        }

-        throw new \UnexpectedValueException('Object not found.');
+        return $this->data[$hash];


This change brings no value.

This approach is about error exception first.

It's about change the order of conditions.

I got that, but what's the point? Please don't spent time on nitpick like this :)

Ok. I will not spend time about this change.

BenMorel · 2018-12-01T10:09:30Z

src/ObjectStorage.php

-        }
-
-        return null;
+        return $this->data[$hash] ?? null;


You can keep this change alone.

How about reverting this due to your previous comment.

This is an isset($x) ? $x : null, so conceptually equivalent to $x ?? null. I can accept this change.

Enhance conditions

41b6863

BenMorel reviewed Dec 1, 2018

View reviewed changes

Change into original condition approach

7fee5ac

Comments

Conversation

peter279k commented Dec 1, 2018

Changed log

Uh oh!

coveralls commented Dec 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

peter279k Dec 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

coveralls commented Dec 1, 2018 •

edited

Loading

peter279k Dec 1, 2018 •

edited

Loading